Initial changes for saving product images upload statuses in UserDefaults #15196
Conversation
…es in UserDefaults - Modify `ProductImageStatus.swift` to add Codable conformance and additional properties - Create `ProductImagesUserDefaultsStatuses.swift` for managing product image statuses in User Defaults - Move `ProductOrVariationID` in a separate file to define a type-safe identifier for products and variations - Update `Model.swift` to include `ProductOrVariationID` typealias
- Update `ProductImagesCollectionViewDataSource.swift` to include siteID and productID in `ProductImageStatus` cases. - Modify `ProductImagesHeaderTableViewCell.swift` to handle additional parameters in `ProductImageStatus`. - Adjust `Product+Media.swift` to pass siteID and productID when mapping images to `ProductImageStatus`. - Change `ProductImageActionHandler.swift` to incorporate siteID and productID in `ProductImageStatus` handling and updating. - Update `ProductImageStatus+Extension.swift` to manage new parameters in `ProductImageStatus`. - Amend `ProductImagesCollectionViewController.swift` to handle siteID and productID in status cases. - Modify `ProductImagesSaver.swift` to use siteID and productID in status comparison and updates. - Adjust `ProductImagesViewController.swift` to manage `ProductImageStatus` with siteID and productID.
… all statuses without the correct value are now updated. This ensures that even if the statuses were initially created with a nil or outdated productID, they will be updated to the new remoteProductID.
…uploading` and `uploadFailure` cases - Modify `ProductImagesUserDefaultsStatuses.swift` to handle optional `productID` in filtering statuses
pmusolino
added
feature: product details
Generated by 🚫 Danger |
|
|
pmusolino
changed the title
…e for variations.
|
Version |
…e-product-image-upload-statuses-in-user-defaults
…iationID` in `uploading` and `uploadFailure` cases - Update unit tests
…aving_successfully
…pending upload, then trigger the media upload after adding the observer.
…ic. Instead of direct PHAsset comparison, compare localIdentifier to ensure accurate detection and state updates from "uploading" to "remote." This resolves the issue where hasUnsavedChangesOnImages(...) incorrectly returns true after saving, fixing related test failures.
…id default `ServiceLocator` usage.
There was a problem hiding this comment.
Hi Paolo, this PR is really large so it was hard to review. I'm leaving all my comments here first before testing the app.
Next time, it'd be great to split PRs into smaller pieces and have thorough tests for each change rather than wrapping everything in one go. For example:
- Part 1: Create copies of
ProductImageStatus,ProductOrVariationIDin the Networking layer and add tests for the codable conformance. - Part 2: Add the storage for product image status and unit test it.
- Part 3: Replace the networking types in the UI layer.
...lasses/ViewRelated/Products/Cells/Product Images/ProductImagesCollectionViewDataSource.swift
Outdated
Show resolved
Hide resolved
| // If the product is a variation, substitute the existing status with the new uploading status. | ||
| // Otherwise, if a standard product, append a new status. | ||
| if case .variation = self.productOrVariationID { | ||
| self.productImageStatuses = [uploadingStatus] | ||
| } else { | ||
| self.productImageStatuses = [uploadingStatus] + self.productImageStatuses | ||
| } |
There was a problem hiding this comment.
While this check is correct as we allow only one image for variations, the logic should not be part of ProductImageActionhandler IMO. The UI should already take care of disabling uploading more than one image for variations. I'd suggest restoring the previous logic.
There was a problem hiding this comment.
I think this is a safeguard that makes sense to keep. The ProductImageActionHandler is one of the central pieces that manages and tracks image upload statuses. Even if the UI intends to allow only one image for variations, the handler is the final arbiter. And in fact, reverting the code generate a crash because the UI is not managing in a proper way the image statuses, replaing the current images showed. This means that if you upload an image in a variation, save it, then you try to replace it, you can expect a crash because in the end the images will be two (the remote one + the new one).
There was a problem hiding this comment.
Are you suggesting that the previous logic is causing a crash on trunk? I tried as you suggested but couldn't reproduce the crash.
I think the crash should not happen because when you replace an image, the remote image is discarded from the status list.
There was a problem hiding this comment.
Tagging @pmusolino in case this message got lost is the depth of discussions here. Could you clarify my last question above please?
There was a problem hiding this comment.
@itsmeichigo No, I meant that given how the status search now works with siteID and productID, this looks like a necessary change in this PR. In trunk, everything should work as before.
WooCommerce/Classes/ViewRelated/Products/Media/ProductImageActionHandler.swift
Outdated
Show resolved
Hide resolved
WooCommerce/Classes/ViewRelated/Products/Media/ProductImageStatus+Extension.swift
Outdated
Show resolved
Hide resolved
WooCommerce/Classes/ViewRelated/Products/Media/ProductImageStatus+Extension.swift
Outdated
Show resolved
Hide resolved
| /// Save product image upload statuses in User Defaults. | ||
| /// This class is declared in the Networking layer because it will also be accessed by the background URLSession operations. | ||
| /// | ||
| final class ProductImagesUserDefaultsStatuses { |
There was a problem hiding this comment.
Please consider renaming this to ProductImageStatusStorage and injecting user defaults into its initializer for testability.
There was a problem hiding this comment.
This file has been extensively revised in a future PR, where UserDefaults is also injected for testability. Anyway, the suggested name seems interesting to me. Since this file has been thoroughly revisited in future PRs, I will rename it there. 👍
| case "uploadFailure": | ||
| let asset = try container.decode(ProductImageAssetType.self, forKey: .asset) | ||
| let errorMessage = try container.decode(String.self, forKey: .error) | ||
| let error = NSError(domain: "ProductImageStatus", code: 0, userInfo: [NSLocalizedDescriptionKey: errorMessage]) |
There was a problem hiding this comment.
Looks like we'll lose information about the error here. The information is necessary because we're showing details about the upload failures. Please also retain the domain and error code while encoding/decoding.
There was a problem hiding this comment.
This has been handled in a separate PR, but I'm applying the changes from that PR in this one. 👍
| try container.encode("uploading", forKey: .type) | ||
| try container.encode(asset, forKey: .asset) | ||
| try container.encode(siteID, forKey: .siteID) | ||
| try container.encodeIfPresent(productID, forKey: .productID) |
There was a problem hiding this comment.
Any reason why encodeIfPresent is necessary? I think productID is non-optional here.
There was a problem hiding this comment.
Correct. In fact, in a following PR, this has been changed. Like above, I'm applying the changes from that PR in this one.
Networking/Networking/ProductImageInBackground/ProductImageStatus.swift
Outdated
Show resolved
Hide resolved
| if let filterProductID = productID { | ||
| return sID == siteID && pID == filterProductID | ||
| } else { | ||
| return sID == siteID && pID == nil |
There was a problem hiding this comment.
Product ID is non-optional in image statuses so this check will likely fail.
There was a problem hiding this comment.
As mentioned in the PR description, I suggested not reviewing this file since it is not used and, above all, it has been deeply modified in a future PR (specifically in this one, which will be split into multiple PRs due to its size). This code no longer exists there 👍 so you can ignore it for the moment.
There was a problem hiding this comment.
If this file is not ready, please do not commit it into a PR like this. It would be hard for me to approve the PR because not everything is working and I cannot guarantee the quality of the code to be merged.
…tID` in `ProductImageUploaderTests.swift`
- Add new cases for `errorDomain`, `errorCode`, and `errorUserInfo` in `ProductImageStatus.swift`. - Modify decoding logic to handle detailed error information. - Update encoding logic to include `errorDomain`, `errorCode`, and `errorUserInfo`. - Change `productID` to be always encoded in `uploading` and `uploadFailure` cases.
…omparison in ProductImageActionHandler
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughThe pull request introduces new Swift files and updates to existing ones to enhance product image status management. Three new files—defining enums and a status management class—are organized in a new group within the Xcode project. Changes extend the image status types by adding parameters (siteID and productID) and refactor existing methods, switch cases, and tests accordingly. The pull request also removes redundant enum definitions and updates project file references and public type aliases to align with the new structure. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as ViewController
participant Uploader as ProductImageActionHandler
participant Storage as ProductImagesUserDefaultsStatuses
participant Backend as Server
UI->>Uploader: Initiate image upload (asset, siteID, productID)
Uploader->>Storage: addStatus(.uploading(asset, siteID, productID))
Uploader->>Backend: Upload asset
Backend-->>Uploader: Upload response (success/failure)
alt Success
Uploader->>Storage: updateStatus(.remote(image, siteID, productID))
else Failure
Uploader->>Storage: updateStatus(.uploadFailure(asset, error, siteID, productID))
end
Uploader-->>UI: Notify upload result
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
pmusolino
changed the title
pmusolino
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (1)
WooCommerce/WooCommerceTests/ViewRelated/Products/Media/ProductImageUploaderTests.swift (1)
1-718: 🛠️ Refactor suggestionMissing unit tests for the new UserDefaults functionality.
According to the PR objectives, this change introduces the foundation for storing product image upload statuses in UserDefaults. However, there are no tests directly verifying this functionality. Consider adding specific tests that check if statuses are properly saved to and retrieved from UserDefaults.
Would you like me to generate a sample test case for verifying the UserDefaults functionality?
🧹 Nitpick comments (13)
WooCommerce/Classes/ViewRelated/Products/Cells/Product Images/ProductImagesHeaderTableViewCell.swift (1)
79-79: Consider utilizing the additional parameters in error handlingSimilar to the
.remotecase, the.uploadFailurecase now includessiteIDandproductIDparameters that are being ignored. Since these provide context about where the failure occurred, consider whether these should be passed to theonFailedUploadSelectedhandler to improve error reporting or recovery.-case let .uploadFailure(asset, error, _, _): - onFailedUploadSelected?(asset, error) +case let .uploadFailure(asset, error, siteID, productID): + onFailedUploadSelected?(asset, error, siteID, productID)Note: This would require updating the
onFailedUploadSelectedclosure definition and all implementations.Networking/Networking/ProductImageInBackground/ProductOrVariationID.swift (2)
7-14: Consider adding a productID accessor methodThe
idproperty returns different values based on the enum case - product ID for.productand variation ID for.variation. For scenarios where you specifically need the product ID regardless of case, consider adding a dedicated accessor.+/// Returns the product ID for both product and variation types +public var productID: Int64 { + switch self { + case .product(let id): + return id + case .variation(let productID, _): + return productID + } +}
28-40: Add error handling for incomplete JSONThe decoder implementation should handle cases where the required fields are missing or have incorrect types. Consider adding more robust error handling or using default values where appropriate.
public init(from decoder: Decoder) throws { let container = try decoder.container(keyedBy: CodingKeys.self) - let caseType = try container.decode(CaseType.self, forKey: .type) + // Handle missing or invalid type with a custom error message + guard let caseType = try? container.decode(CaseType.self, forKey: .type) else { + throw DecodingError.dataCorruptedError( + forKey: .type, + in: container, + debugDescription: "Invalid or missing ProductOrVariationID type" + ) + } switch caseType { // ... rest of the implementation remains the sameNetworking/Networking/ProductImageInBackground/ProductImageStatus.swift (3)
19-19: Ensure robust error encoding/decoding.Currently, the code assumes the
NSError.userInfois[String: String]?. This might cause data loss if the error contains more complex information. Consider either serializing the entireuserInfomore flexibly or storing a simplified representation of the error.
127-135: Check concurrency and availability of PHAsset fetch.Decoding a
phAssetby synchronously callingPHAsset.fetchAssetscan block if performed on the main thread. Consider offloading this fetch or marking the decoding process to run on a background thread to improve responsiveness.
160-166: Evaluate PNG-only serialization for UIImages.Relying on
pngData()might lead to large serialized data. Using JPEG compression or other strategies may reduce payload size while maintaining acceptable quality.WooCommerce/Classes/ViewRelated/Products/Media/ProductImagesCollectionViewController.swift (2)
90-92: Updated pattern matching with unused parameters.The pattern matching has been correctly updated to match the new
ProductImageStatusenum cases while preserving the existing functionality.Consider making the parameter names explicit for better code readability, even though they're not used in this context:
-case .remote(let image, _, _): +case .remote(let image, let siteID, let productID):This would make it clearer to future developers what these parameters represent, though it's perfectly fine to use underscores for unused parameters.
90-106: Consider addressing unused parameters.All switch cases use underscores for unused parameters. While this is standard Swift practice, a previous reviewer suggested omitting unused params.
You could refactor the
ProductImageStatusenum to separate the status from its metadata, or create helper methods that extract only needed information.Networking/Networking/ProductImageInBackground/ProductImagesUserDefaultsStatuses.swift (3)
9-19: Consider concurrency protection.
Rewriting and re-encoding the entire list inaddStatus/removeStatusmay cause data loss if multiple threads call these methods simultaneously. A dedicated queue or locking mechanism can help ensure atomic writes.
25-36: Cautious approach to decoding errors.
Returning an empty list on decode failure is safe but silently discards data. Consider logging additional context or providing fallback recovery steps if decoding is critical.
62-69: Ensure large data sets do not degrade performance.
Serializing and storing many statuses in user defaults may become expensive. If the list grows, consider an alternative storage strategy or routine cleanup to maintain efficiency.WooCommerce/Classes/ViewRelated/Products/Media/ProductImagesSaver.swift (2)
132-136: Pattern matching needs to utilize the siteID and productID parametersThe case pattern discards the siteID and productID parameters with underscore placeholders, but doesn't use them in the function. Consider either using named variables to improve readability or documenting why these values are intentionally ignored.
-case .uploading(let uploadingAsset, _, _): +case .uploading(let uploadingAsset, let _, let _):Or better yet, if you need to use these values later:
-case .uploading(let uploadingAsset, _, _): +case .uploading(let uploadingAsset, let siteID, let productID):
132-137: Consider handling extracted parameters differently.The switch pattern extracts unused parameters with underscores, which is standard Swift practice but adds complexity.
A previous reviewer suggested omitting unused params. You could refactor this pattern to avoid needing to handle these unused parameters, perhaps through helper methods.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
Networking/Networking.xcodeproj/project.pbxproj(6 hunks)Networking/Networking/ProductImageInBackground/ProductImageStatus.swift(1 hunks)Networking/Networking/ProductImageInBackground/ProductImagesUserDefaultsStatuses.swift(1 hunks)Networking/Networking/ProductImageInBackground/ProductOrVariationID.swift(1 hunks)RELEASE-NOTES.txt(1 hunks)WooCommerce/Classes/ServiceLocator/ProductImageUploader.swift(1 hunks)WooCommerce/Classes/ViewRelated/Products/Cells/Product Images/ProductImagesCollectionViewDataSource.swift(1 hunks)WooCommerce/Classes/ViewRelated/Products/Cells/Product Images/ProductImagesHeaderTableViewCell.swift(1 hunks)WooCommerce/Classes/ViewRelated/Products/Media/Product+Media.swift(1 hunks)WooCommerce/Classes/ViewRelated/Products/Media/ProductImageActionHandler.swift(5 hunks)WooCommerce/Classes/ViewRelated/Products/Media/ProductImageStatus+Extension.swift(3 hunks)WooCommerce/Classes/ViewRelated/Products/Media/ProductImagesCollectionViewController.swift(2 hunks)WooCommerce/Classes/ViewRelated/Products/Media/ProductImagesSaver.swift(2 hunks)WooCommerce/Classes/ViewRelated/Products/Media/ProductImagesViewController.swift(1 hunks)WooCommerce/WooCommerce.xcodeproj/project.pbxproj(8 hunks)WooCommerce/WooCommerceTests/Mocks/MockProductImageActionHandler.swift(1 hunks)WooCommerce/WooCommerceTests/ViewRelated/Products/Edit Product/ProductVariationFormViewModel+ImageUploaderTests.swift(2 hunks)WooCommerce/WooCommerceTests/ViewRelated/Products/Media/ProductImageActionHandlerTests.swift(13 hunks)WooCommerce/WooCommerceTests/ViewRelated/Products/Media/ProductImageStatus+HelpersTests.swift(5 hunks)WooCommerce/WooCommerceTests/ViewRelated/Products/Media/ProductImageUploaderTests.swift(31 hunks)WooCommerce/WooCommerceTests/ViewRelated/Products/Media/ProductImagesSaverTests.swift(6 hunks)Yosemite/Yosemite/Model/Model.swift(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- RELEASE-NOTES.txt
👮 Files not reviewed due to content moderation or server errors (8)
- WooCommerce/Classes/ViewRelated/Products/Cells/Product Images/ProductImagesHeaderTableViewCell.swift
- WooCommerce/Classes/ViewRelated/Products/Media/Product+Media.swift
- WooCommerce/WooCommerceTests/ViewRelated/Products/Edit Product/ProductVariationFormViewModel+ImageUploaderTests.swift
- Networking/Networking/ProductImageInBackground/ProductOrVariationID.swift
- WooCommerce/WooCommerceTests/ViewRelated/Products/Media/ProductImageActionHandlerTests.swift
- WooCommerce/Classes/ViewRelated/Products/Media/ProductImagesSaver.swift
- WooCommerce/WooCommerceTests/ViewRelated/Products/Media/ProductImageStatus+HelpersTests.swift
- WooCommerce/Classes/ViewRelated/Products/Media/ProductImagesCollectionViewController.swift
🔇 Additional comments (74)
WooCommerce/Classes/ViewRelated/Products/Media/ProductImagesViewController.swift (1)
29-29: Update pattern matching to accommodate additional parametersThe case pattern for
.remotehas been updated to include two additional parameters (marked with underscores), aligning with the enhancedProductImageStatusenum that now includessiteIDandproductID.Yosemite/Yosemite/Model/Model.swift (1)
247-249: Added new type aliases for product image functionalityThese new type aliases align with the PR objective of relocating image-related types to the Networking layer for better access during background uploads, which is essential for supporting the storage of product image upload statuses in UserDefaults.
WooCommerce/WooCommerceTests/Mocks/MockProductImageActionHandler.swift (1)
6-8: Added imports for relocated enumsAdded imports for the
ProductImageStatus,ProductImageAssetType, andProductOrVariationIDenums that have been moved to the Networking layer. This ensures the mock handler can work with the same types as the actual implementation.WooCommerce/WooCommerce.xcodeproj/project.pbxproj (1)
605-605: Project structure organization aligns with PR objectivesThe renaming of
ProductImageStatus.swifttoProductImageStatus+Extension.swiftand addition ofMockProductImageActionHandler.swiftproperly support the architectural changes needed for background image uploads. These references are consistently updated throughout the project file.Also applies to: 1412-1412, 3821-3821, 4572-4572, 7455-7455, 9960-9960, 16253-16253, 17790-17790
WooCommerce/Classes/ViewRelated/Products/Cells/Product Images/ProductImagesHeaderTableViewCell.swift (5)
75-75:❓ Verification inconclusive
Update case pattern matching to handle new parameters correctly
The case matching has been updated to support additional parameters in
ProductImageStatus.remote, but the new parameters are ignored with_placeholders instead of being explicitly named. Consider whether these parameters provide useful context that should be passed toonImageSelectedor other handlers.
🏁 Script executed:
#!/bin/bash # Check how onImageSelected is used in the codebase rg "onImageSelected\??\(" -A 3Length of output: 2826
Clarify Handling of Additional Parameters in the
.remoteCaseIn
ProductImagesHeaderTableViewCell.swift(around line 75), the.remotecase is currently written as:case .remote(let image, _, _):The new parameters introduced in
ProductImageStatus.remoteare being ignored with_placeholders. Since theonImageSelectedcallback in this file is invoked withimageandindexPath(as seen in the surrounding code), please consider the following:
- Evaluate Parameter Relevance: Determine whether the additional parameters provide useful context (e.g., metadata or state information) that should be forwarded to
onImageSelectedor used in another way.- Improve Code Clarity: If these parameters are meant to be used later, name them descriptively instead of using
_. Conversely, if they are intentionally ignored, please add an inline comment to document the decision.
75-75: Code correctly updated for additional parameters.The pattern matching for
.remotecase has been updated to handle the newsiteIDandproductIDparameters, correctly extracting only theimagewhile ignoring the additional values with underscores.
79-79: Code correctly updated for additional parameters.The pattern matching for
.uploadFailurecase has been updated to handle the newsiteIDandproductIDparameters, correctly extracting only the necessaryassetanderrorwhile ignoring the additional values.
75-75: Update case pattern to include new parametersThe case pattern for
.remotehas been updated to include additional parameters forsiteIDandproductID. These parameters are currently not used in this context but are correctly ignored with wildcards.
79-79: Update case pattern to include new parametersThe case pattern for
.uploadFailurehas been updated to include additional parameters forsiteIDandproductID. These parameters are correctly ignored using wildcards since they're not needed in this context.WooCommerce/Classes/ViewRelated/Products/Media/Product+Media.swift (5)
6-6: Updated image status construction to include site and product IDsThe changes correctly update the
ProductImageStatus.remoteinitialization to includesiteIDandproductID, which is essential for the background upload functionality. The implementation consistently applies this pattern to bothProductFormDataModelandProductextensions.Also applies to: 12-12
6-6: Implementation correctly updated with required parameters.The
imageStatusescomputed property has been updated to include the new required parameters (siteIDandproductID) when creating eachProductImageStatus.remoteinstance. This aligns with the enhanced structure for supporting background image uploads.
12-12: Implementation correctly updated with required parameters.The
imageStatusescomputed property in theProductextension has been properly updated to include the requiredsiteIDandproductIDparameters, consistent with the changes in theProductFormDataModelextension.
6-6: LGTM: Add site and product identification to remote image statusesThe
imageStatusesproperty now correctly includes thesiteIDandproductIDparameters when creating remote image statuses, which is essential for supporting background image uploads as mentioned in the PR objectives.
12-12: LGTM: Add site and product identification to remote image statusesSimilar to the ProductFormDataModel extension, this implementation correctly includes the
siteIDandproductIDparameters when creating remote image statuses.WooCommerce/WooCommerceTests/ViewRelated/Products/Edit Product/ProductVariationFormViewModel+ImageUploaderTests.swift (6)
11-12: Constants added for new required test parametersAdding the
siteIDandproductIDconstants properly sets up the test environment for the updatedProductImageStatusimplementation. Using static values here is appropriate for unit testing.
28-30: Updated mock handler initialization with required parametersThe test has been correctly updated to pass the new required parameters to the
MockProductImageActionHandlerinitialization, ensuring that the test reflects the updated function signatures.
11-12: Test properties added correctly.Added properties for
siteIDandproductIDwith appropriate test values. TheproductIDcorrectly uses the newProductOrVariationIDenum to create a product identifier.
28-30: Test correctly updated with new parameters.The
MockProductImageActionHandlerinitialization has been updated to include the new required parameters (siteIDandproductID) for the.uploadingstatus, ensuring the tests properly represent the updated implementation.
11-12: LGTM: Add test properties for site and product identificationThese properties provide the necessary context for testing with the updated ProductImageStatus enum that now requires siteID and productID parameters.
28-30: LGTM: Update test case with new parametersThe mock ProductImageActionHandler initialization has been properly updated to include the siteID and productID parameters in the .uploading status.
Networking/Networking/ProductImageInBackground/ProductOrVariationID.swift (3)
1-54: Add tests for Codable conformanceThe
ProductOrVariationIDenum is well-designed with proper Codable implementation for both cases. However, as mentioned in previous feedback, please add unit tests to confirm both encoding and decoding work as expected, especially given the complexity of handling different enum cases.#!/bin/bash # Check if there are any unit tests for this new file rg -l "ProductOrVariationID" --type swift | grep -i test
1-54: Well-structured enum for representing product and variation IDs.The implementation looks solid with:
- Clear cases for both product and variation IDs
- A computed
idproperty that handles both cases appropriately- Proper Codable implementation with custom encoding and decoding
As mentioned in a previous review, since this enum conforms to
Codable, please ensure you add tests to verify that both encoding and decoding work correctly for all cases.What are best practices for testing Swift Codable implementation?
1-54: Add unit tests for Codable implementationThe implementation of this new enum looks good. It provides a structured way to handle product and variation identifiers and properly implements the Codable protocol for persistence in UserDefaults.
However, as noted in a previous review, you should add unit tests to verify that both encoding and decoding work as expected for this enum.
WooCommerce/Classes/ViewRelated/Products/Media/ProductImageStatus+Extension.swift (3)
8-8: Nit: Unused parameters in.remote.It looks like the second and third parameters are not used here. If these parameters aren’t necessary yet, consider removing them to avoid confusion. If they’re intended for future usage, add a comment explaining their purpose.
51-51: Nit: Unused parameters in.uploading.Similar to the
.remotecase, the second and third parameters are currently unused. Removing them or documenting their expected usage helps keep the code clean and maintainable.
60-60: Consider leveraging unused parameters for the drag identifier.If the additional parameters in
.remoterefer to siteID or productID, incorporating them into the drag item identifier could improve uniqueness across multiple sites or products. If they’re truly unneeded here, remove them or add a comment clarifying their future intent.Networking/Networking.xcodeproj/project.pbxproj (3)
2701-2709: Well-organized project structure for the new feature.Creating a dedicated group for the image background processing files (
ProductImageStatus.swift,ProductOrVariationID.swift, andProductImagesUserDefaultsStatuses.swift) is a good organizational practice. This makes the project structure more maintainable by keeping related files together, especially as this feature will be expanded in future PRs.
2944-2944: Appropriate placement of the new group in the project hierarchy.The placement of the
ProductImageInBackgroundgroup at the root level of the project is suitable for its functionality, making it easily accessible for future development while keeping it properly encapsulated.
482-484: Complete and consistent project file references.All required project file references for the new Swift files have been properly added to the necessary sections of the project file (PBXBuildFile, PBXFileReference, and in build phases). This ensures the files will be correctly included in the build process.
Also applies to: 1696-1698, 5333-5333, 5378-5378, 5385-5385
Networking/Networking/ProductImageInBackground/ProductImageStatus.swift (3)
8-8: Add unit tests for Codable conformance.Previous review feedback (lines 8-8) requested tests for encoding/decoding the
ProductImageStatusenum. These tests are still missing.
95-99: Revisit equality comparison for errors.Using
(lError as NSError) == (rError as NSError)may yield false negatives when bridgingErrortoNSErrorif underlying data changes. Consider storing and comparing only a domain, code, and standardized userInfo keys to avoid unexpected mismatches.
107-107: Add unit tests for Codable conformance.Similar to
ProductImageStatus,ProductImageAssetTypealso now conforms toCodable. Ensure that round-trip encoding/decoding tests validate all associated values.WooCommerce/Classes/ServiceLocator/ProductImageUploader.swift (1)
7-9: Imports look good.The newly added imports for
ProductImageStatus,ProductImageAssetType, andProductOrVariationIDcorrectly reflect the refactor to manage these types in the Yosemite module.WooCommerce/Classes/ViewRelated/Products/Cells/Product Images/ProductImagesCollectionViewDataSource.swift (1)
54-56: Omit unused parameters or handle them if necessary.The switch uses
_forsiteIDandproductID. If they are not needed, consider removing these parameters from the enum or removing the placeholders to avoid confusion.Also applies to: 63-63
WooCommerce/WooCommerceTests/ViewRelated/Products/Media/ProductImageActionHandlerTests.swift (5)
11-12: Appropriate introduction of new properties for test setup.The addition of
siteIDandproductIDproperties provides the necessary test context for the updatedProductImageStatusmodel which now requires these identifiers.
14-14: Good test naming update for consistency.The test method naming has been updated from camelCase to snake_case, which aligns better with Swift unit testing conventions and improves readability.
401-441: Good test coverage for product ID propagation.This new test is crucial as it verifies that the
updateProductIDmethod correctly propagates changes to existing upload statuses. This ensures that when a product transitions from local to remote state (getting a server-assigned ID), all associated uploads are updated accordingly.
401-441: Good implementation of test for product ID updates!The new test verifies that updating a product ID properly propagates to existing upload statuses, which is crucial for maintaining data consistency when switching from a local to remote product ID.
401-441: Good implementation of product ID update test.This test correctly verifies that the product ID is properly updated in existing upload statuses when
updateProductIDis called.Consider adding a brief comment explaining the significance of updating from local to remote product ID to make the test purpose clearer.
WooCommerce/Classes/ViewRelated/Products/Media/ProductImagesSaver.swift (4)
132-133: Correctly updated pattern matching for new parameters.The pattern matching has been updated to accommodate the additional parameters in the
uploadingcase, allowing the code to continue working with the new enum structure.
151-151: Required parameters added to remote image status.The implementation now correctly sets the
siteIDandproductIDwhen creating a remote image status, which is essential for tracking these images in UserDefaults for background uploads.
150-152: Good implementation of site and product tracking in ProductImageStatus updateThe status now properly includes siteID and productID parameters when updating to remote state, which is necessary for consistent background uploads.
150-152: Added new parameters correctly.The
updateProductImageStatusmethod has been updated to include the necessarysiteIDandproductIDparameters in the remote status.WooCommerce/WooCommerceTests/ViewRelated/Products/Media/ProductImageStatus+HelpersTests.swift (5)
9-10: Well-defined test properties for consistent tests.Good practice to define these properties once at the class level rather than repeating the values in each test case, enhancing maintainability.
16-16: Comprehensive test updates for the modified enum cases.All test cases have been consistently updated to include the required
siteIDandproductIDparameters across various scenarios, ensuring the helper methods continue to work correctly with the enhanced data model.Also applies to: 25-26, 41-42, 50-51, 61-61, 74-74
9-10: Good addition of test properties for consistent testingThe addition of siteID and productID properties enables consistent testing across all test methods.
9-10: Good test setup with new parameters.The addition of the
siteIDandproductIDproperties as class properties makes the tests cleaner and easier to update if values need to change.
41-42: Properly updated test cases.The test cases correctly use the new parameters required by the
ProductImageStatusenum.WooCommerce/Classes/ViewRelated/Products/Media/ProductImagesCollectionViewController.swift (7)
99-100: Properly updated pattern matching for upload failures.The upload failure case pattern has been updated to include all new parameters while preserving existing functionality.
189-190: Correctly modified failure case handling.The
didSelectItemAtmethod has been updated to work with the new parameter structure while maintaining the same behavior.
90-91: Unused parameters in remote case patternThe case pattern ignores siteID and productID with underscores, but doesn't document why. Consider adding a comment explaining why these values are intentionally ignored or use named variables for clarity.
92-98: Unused parameters in uploading case patternSimilar to the remote case, the siteID and productID parameters are ignored without explanation.
99-106: Unused parameters in uploadFailure case patternThe case pattern has four parameters but two are ignored with underscores. Consider adding a comment explaining why or use named variables for clarity.
189-191: Consistent pattern matching in didSelectItemAtThe error handling logic for uploadFailure correctly extracts just the relevant parameters (asset and error) and ignores siteID and productID that aren't needed in this context.
189-190: Correctly updated case pattern with error parameter.This switch case correctly extracts the necessary parameters including the error.
WooCommerce/WooCommerceTests/ViewRelated/Products/Media/ProductImagesSaverTests.swift (6)
47-47: Well-structured test assertion.
No functional issue stands out. IncludingsiteIDandproductIDin the uploading status enhances clarity of the test scenario.
88-88: Good inclusion of identifiers in the status check.
Maintaining consistency in these tests helps verify logic across different scenarios.
144-144: Consistent usage of uploading status.
This matches the approach used in similar test cases for ensuring correctness after a product save attempt fails.
159-161: Verify correctness of product ID in a variation test.
This test is named for variation handling, but the uploading status references.product(id: productID)instead of.variation(...). Verify that this mismatch is intentional:- let actionHandler = MockProductImageActionHandler(productImageStatuses: [.uploading(asset: asset, siteID: siteID, productID: .product(id: productID))]) + let actionHandler = MockProductImageActionHandler(productImageStatuses: [.uploading(asset: asset, siteID: siteID, productID: .variation(productID: productID, variationID: 134))])
252-254: Accurate use of variation product ID.
This status properly reflects.variation(...), ensuring the test exercises variation-specific logic.
282-282: Product scenario coverage maintained.
Including.product(id: productID)consistently verifies site-specific logic for standard products.Networking/Networking/ProductImageInBackground/ProductImagesUserDefaultsStatuses.swift (1)
38-56: Clarify handling ofproductID == nil.
The filter case implies a scenario where productID can be absent, but from usage elsewhere, productID often appears mandatory. Confirm that ignoring statuses is intended ifproductIDis nil.WooCommerce/Classes/ViewRelated/Products/Media/ProductImageActionHandler.swift (7)
165-167: Clear representation of remote statuses.
Storing thesiteIDandproductOrVariationIDensures that future retrieval or updates remain unambiguous.
178-186: Variation vs. standard product logic is well-delineated.
Replacing the entire status array for variations vs. appending for products is well-structured. Ensure calling code accounts for this difference in user flows.
259-268: Correct mapping of upload statuses on product ID update.
This approach systematically replaces the local ID with the remote one. Ensure that downstream references are updated to avoid stale IDs.
279-283: Strict matching for remote images.
Filtering by both siteID and productOrVariationID helps prevent accidental removal of unrelated images.
317-321: Properly scoped index search.
Matchingasset,siteID, andproductOrVariationIDinindex(of:)ensures precise status identification. Reduces risk of collisions if multiple uploads share the same asset reference.
329-329: Effective success-state update.
Moving from.uploadingto.remoteis clearly handled, and notifying observers is well-placed here.Also applies to: 334-334
338-338: Thoughtful error handling.
Transitioning from.uploadingto.uploadFailurenotifies all listeners and retains enough context to troubleshoot failures.Also applies to: 342-342
WooCommerce/WooCommerceTests/ViewRelated/Products/Media/ProductImageUploaderTests.swift (5)
27-27: Consider usingproductIDdirectly as suggested in previous review.There's an inconsistency in how the product ID is referenced. In some places,
productIDis used directly, while in others,.product(id: productID.id)is used, which seems redundant sinceproductIDalready contains this information.- productOrVariationID: .product(id: productID.id), + productOrVariationID: productID,Also applies to: 40-40, 44-44, 49-49
85-87: LGTM! Improved code formatting.The if-statement block now follows proper Swift style guidelines with braces on separate lines.
72-72: Verify fake product creation with ID mapping.When creating fake test products, the productID.id is now used. Make sure this properly maps to what the model expects and that all product-related functionality works correctly with this change.
Also applies to: 632-632
16-19:✅ Verification successful
Verify the implementation of MockProductImagesProductIDUpdater.
The tests now use a new dependency
MockProductImagesProductIDUpdater. Ensure that this mock properly implements all required functionality and that it faithfully reproduces the behavior of the real implementation for testing.Also applies to: 190-193, 225-228
🏁 Script executed:
#!/bin/bash # Check the implementation of MockProductImagesProductIDUpdater # Search for the mock class definition rg -A 10 "class MockProductImagesProductIDUpdater" --type swiftLength of output: 1205
Verification Complete: Mock Implementation Meets Testing Requirements
The implementation of
MockProductImagesProductIDUpdater(located inWooCommerce/WooCommerceTests/Mocks/MockProductImagesProductIDUpdater.swift) correctly conforms toProductImagesProductIDUpdaterProtocol. The mock sets theupdateImageProductIDWasCalledflag and returns a fakeMediaas expected, which appears sufficient for the testing scenarios in use (including the instances at lines 16-19, 190-193, and 225-228). No additional changes are required.
195-197:✅ Verification successful
Ensure ProductImageStatus properly includes site and product ID parameters.
The ProductImageStatus enum has been updated to include siteID and productID parameters. Verify that these parameters are properly handled in the actual implementation, especially for background uploads.
Also applies to: 232-234
🏁 Script executed:
#!/bin/bash # Check the implementation of ProductImageStatus # Search for the enum definition and its cases rg -A 20 "enum ProductImageStatus" --type swiftLength of output: 2253
Background Uploads: Confirm Parameter Handling
The enum definition of
ProductImageStatus(located inNetworking/Networking/ProductImageInBackground/ProductImageStatus.swift) shows that all cases, including.remoteand.uploading, now correctly require and handle bothsiteIDandproductIDparameters. The test instantiations inWooCommerce/WooCommerceTests/ViewRelated/Products/Media/ProductImageUploaderTests.swiftfor lines 195–197 and 232–234 match this updated implementation.No further action is required here, but please verify that all background upload flows correctly propagate and utilize these parameters where necessary.
Networking/Networking/ProductImageInBackground/ProductImagesUserDefaultsStatuses.swift
Outdated
Show resolved
Hide resolved
WooCommerce/WooCommerceTests/ViewRelated/Products/Media/ProductImageUploaderTests.swift
Outdated
Show resolved
Hide resolved
WooCommerce/WooCommerceTests/ViewRelated/Products/Media/ProductImageActionHandlerTests.swift
Outdated
Show resolved
Hide resolved
WooCommerce/Classes/ViewRelated/Products/Media/ProductImagesCollectionViewController.swift
Show resolved
Hide resolved
…ad of a hardcoded value. - Change `ProductImageUploaderTests.swift` to correctly assign `productID.id` to `remoteProductID`.
There was a problem hiding this comment.
Hey Paolo, I tested the image upload and reordering on this PR and it seems to still work as before. However, it's really hard for me to approve this PR given that the file ProductImagesUserDefaultsStatuses is not ready.
I understand that you are changing the file in a subsequent PR - how about removing it from this branch and adding it only to your subsequent PR? That way, we would not have to merge unfinished code.
… extensively updated later on.
|
Thank you @itsmeichigo for your patience and review. I will split the next PRs into multiple PRs (I have at least 3 in the queue), because there is so much code, and I removed from this one what has been heavily modified in a future PR (in this case |
Description
Part 1 of the changes needed for product image uploads in the background.
UserDefaults. (The actual save will happen in another PR) Ref. pe5sF9-3UcsiteIDandproductID.URLSessionbackground configuration doesn't have access toServiceLocatoror similar classes.URLSessionupload tasks.ProductImagesUserDefaultsStatusesfor storing statuses inUserDefaults.ProductImagesUserDefaultsStatusesdue to significant changes in a subsequent PR.Questions that may arise:
siteIDandproductIDinProductImageStatus?:URLSessiondoesn't allow request updates during execution or scheduling. If the app goes in background, we do not have any power on it.save, updating requests isn't feasible.UserDefaultsand send subsequent requests at the end of every future upload request.Main changes applied in this PR:
ProductImageStatusto the Networking layer for backgroundURLSessionaccess.ProductOrVariationIDenum to Networking layer for consistency.ProductImageStatusandProductImageAssetTypenow conform toCodableforUserDefaultsstorage.ProductImageStatusenum cases (uploading,remote,uploadFailure) now acceptsiteIDandproductID.ProductImagesUserDefaultsStatusesforProductImageStatusstorage.siteIDandproductID.Testing information
RELEASE-NOTES.txtif necessary.Reviewer (or Author, in the case of optional code reviews):
Please make sure these conditions are met before approving the PR, or request changes if the PR needs improvement:
Summary by CodeRabbit